Improve subtitle selection UX: Move "No Subtitles" option to bottom#2523
Improve subtitle selection UX: Move "No Subtitles" option to bottom#2523hrisabhy wants to merge 2 commits intorecloudstream:masterfrom
Conversation
|
The idea to move "No subtitles" to the bottom makes sense from a consistency standpoint, but moving it below the '+ ...' footers is inconsistent with other subtitles. If you want this implemented, the "No subtitles" option would need to be above them. |
|
Hi @fire-light42, I've updated the PR based on your feedback. Changes made:
The footer ordering is now:
|
fire-light42
left a comment
fire-light42
left a comment
There was a problem hiding this comment.
It looks like you are using AI to assist you in creating this pull request. Please read our AI policy and mark your pull request accordingly.
| } | ||
|
|
||
| val noSubtitlesFooter: TextView = | ||
| layoutInflater.inflate(R.layout.sort_bottom_single_choice, null) as TextView |
There was a problem hiding this comment.
Why are you creating a whole new view? We just need simple reordering of what we already have. This creates unnecessary complexity and change of existing logic.
This pull request should be roughly 7 lines changed, not 46.
There was a problem hiding this comment.
Thanks for pointing this out. You were right my earlier change was unnecessarily complex.
I have pushed an update keeping the change minimal and avoided touching existing logic.
Also about the AI part I did initially take some assistance while drafting but I reworked the changes myself to match the project style. I will make sure to follow the AI policy properly and mark it next time.
Let me know if anything still needs adjustment.
|
|
||
| // Cheeky way of getting the view at that position to click it | ||
| // to avoid keeping track of the various footers. | ||
| // getChildAt() gives null :( |
There was a problem hiding this comment.
You are removing valuable comments for no reason. Do not change existing code or comments without justification.
There was a problem hiding this comment.
Got it 👍 I have restored the important comments and avoided changing existing ones unless necessary.
- Moved 'No Subtitles' from top to bottom of adapter - Adjusted subtitle index calculations (removed +1 offset) - Changed condition from <= 0 to >= size for detecting no subtitle selection
hrisabhy
force-pushed
the
fix/subtitle-toggle-ux
branch
from
57f107e to
eab2b07
Compare
fire-light42
left a comment
fire-light42
left a comment
There was a problem hiding this comment.
Very good changes.
However, you need to test your changes before requesting a review.
|
|
||
| val subtitleGroupIndexStart = | ||
| subtitlesGrouped.keys.indexOf(currentSelectedSubtitles?.originalName) + 1 | ||
| subtitlesGrouped.keys.indexOf(currentSelectedSubtitles?.originalName) |
There was a problem hiding this comment.
Now when you select no subtitles the checkmark does not appear, because this index returns -1.
There was a problem hiding this comment.
This should now be fixed.
When No Subtitles was selected, currentSelectedSubtitles became null. On reopening the dialog, indexOf(...) returned -1, so setItemChecked(-1, true) did nothing and the checkmark was not shown.
I now handle the null case explicitly by setting subtitleGroupIndexStart = subtitlesGrouped.size, which points to the No Subtitles item in the adapter (same convention used in the apply logic).
I had tested this earlier, but after your feedback I rechecked and noticed the selection state was not persisting when reopening the dialog. I’ve retested the full flow and it now works as expected.
When "No Subtitles" was selected, currentSelectedSubtitles was null, causing indexOf to return -1 and setItemChecked(-1, true) to silently no-op. Map null to subtitlesGrouped.size (the adapter position of "No Subtitles") to correctly render the checkmark.
2 participants
Problem
The "No Subtitles" option was hidden at the top of the subtitle list, requiring users to scroll up to discover it. This was not intuitive, especially for new users who expected options to be visible or at the bottom.
Solution
sort_bottom_footer_add_choicetosort_bottom_single_choicelayoutChanges
Files modified:
app/src/main/java/com/lagradost/cloudstream3/ui/player/GeneratorPlayer.ktsubtitleGroupIndexStartcalculation (removed +1 offset)app/src/main/res/layout/player_select_source_and_subs.xmlTesting
Tested on Android device with multiple subtitle sources. Verified:
Demo
Before
"No Subtitles" hidden at top, requiring upward scroll
After
"No Subtitles" visible at bottom, scroll indicators show more content
Related
Improves UX for subtitle management without requiring new features or breaking changes.